feat(breaking): migrate Flow to TS; remove some deprecated functions and aliases #881

AugustinLF · 2021-12-06T15:10:34Z

This is the first draft of a PR to switch from flow to TS. The PR includes a green build for typescript and tests. It also includes the deletion of several deprecated APIs as discussed in #877.

There is still a bit left to do, but I'd like to have to have a 👍 before fixing those:

flow types (like we used to do with TS)
Build set up: I'm unsure what strategy we'd like to use to publish a typeless build + expose the TS types, if not I'll try to follow repack's one

Removed functionality:

deprecation messages for all fns deprecated in v2 (getByName, getByType, etc)
waitForElement
shallow
flushMicrotasksQueue

Closes #877

babel.config.js

AugustinLF · 2021-12-06T15:19:54Z

src/__tests__/a11yAPI.test.tsx

      <TouchableOpacity
        accessibilityHint={BUTTON_HINT}
        accessibilityLabel={BUTTON_LABEL}
        accessibilityRole="button"
+        // @ts-ignore - accessibilityStates removed in RN 0.62


Should we consider dropping support for that? Just curious

maybe try accessibilityState={{ selected: true }}

We do support accessibilityState (see below for instance, my question is that since RN 0.62 doesn't support accessibilityStates anymore, should we support it? And if yes, for how long?

Since the oldest React-Native version mentioned in docs is >= 0.61 and I see no clear "minimum supported React Native version" info, I'd assume people can be using RNTL in 0.61~ versions where accessibilityStates prop is still around. Thus I'd keep it if it does not incur big maintenance cost.

AugustinLF · 2021-12-06T15:21:16Z

src/__tests__/a11yAPI.test.tsx

-  expect(
-    disabledSelected && disabledSelected.props.accessibilityStates
-  ).toEqual(['selected', 'disabled']);
+  expect(disabledSelected?.props.accessibilityStates).toEqual([


It runs TS linter rules now, is it something we want? Perhaps we need to make sure that this doesn't have more impact (some JS rules disappearing, or something like that?)

Since it extends callstack ESLint preset I think it's more of a concern of @callstack/eslint-config to maintain parity between the two? I've quickly checked and I don't see glaring differences between JS/TS configs. @thymikee do you think it may be a problem?

AugustinLF · 2021-12-06T15:22:51Z

src/flushMicroTasks.js

-/**
- * Wait for microtasks queue to flush
- */
-export default function flushMicrotasksQueue<T>(): Thenable<T> {


Deleted this one

AugustinLF · 2021-12-06T15:23:17Z

src/helpers/a11yAPI.ts

-  findByLabelText: (string | RegExp, ?WaitForOptions) => FindReturn,
-  findAllByA11yLabel: (string | RegExp, ?WaitForOptions) => FindAllReturn,
-  findAllByLabelText: (string | RegExp, ?WaitForOptions) => FindAllReturn,
+  getByA11yLabel: (label: string | RegExp) => GetReturn;


Should we consider deprecating the aliases?

I guess I would keep them for the time being, as not all a11y queries have their RTL-like counterpart.
@thymikee wdyt?

In my ideal world each query should exists only once (no alias) and have the same behaviour as on web. For instance, we could drop getByA11yLabel and keep getByLabelText, since this one is supported on both platforms.

Let's get rid of aliases for queries that are available on RTL, like ByLabelText for example. It's a simple search-replace change, so no biggie

Removed the *LabelText and *Role aliases.

src/helpers/makeA11yQuery.ts

src/helpers/makeQueries.ts

src/pure.ts

src/waitFor.ts

AugustinLF · 2021-12-06T15:33:32Z

tsconfig.json

@@ -0,0 +1,17 @@
+{


Copied from the repack one, adding jsx support

mdjastrzebski

Great work @AugustinLF! I strongly support moving from Flow to Typescript as a better and de-facto standard type-checking solution.

src/__tests__/a11yAPI.test.tsx

src/helpers/makeA11yQuery.ts

src/helpers/getByAPI.ts

mdjastrzebski · 2021-12-10T09:21:19Z

src/helpers/errors.ts

  }
-  throw new ErrorWithStack(error.message, callsite);
+
+  throw new ErrorWithStack('Something wrong happened when querying', callsite);


Consider something along the lines:

Suggested change

throw new ErrorWithStack('Something wrong happened when querying', callsite);

throw new ErrorWithStack(`Query: caught unknown error type: ${typeof error}, value: ${error?.toString()}`, callsite);

src/helpers/errors.ts

mdjastrzebski · 2021-12-10T09:31:11Z

src/helpers/byDisplayValue.ts

@@ -23,7 +23,10 @@ const getTextInputNodeByDisplayValue = (
      matches(value, nodeValue, normalizer, exact)
    );
  } catch (error) {
-    throw createLibraryNotSupportedError(error);
+    if (error instanceof Error) {


We should not silently ignore the non-Error values. It should be better to just analyse the error type inside the createLibraryNotSupportedError.

mdjastrzebski · 2021-12-10T09:35:13Z

src/helpers/a11yAPI.ts

-  findByLabelText: (string | RegExp, ?WaitForOptions) => FindReturn,
-  findAllByA11yLabel: (string | RegExp, ?WaitForOptions) => FindAllReturn,
-  findAllByLabelText: (string | RegExp, ?WaitForOptions) => FindAllReturn,
+  getByA11yLabel: (label: string | RegExp) => GetReturn;


I guess I would keep them for the time being, as not all a11y queries have their RTL-like counterpart.
@thymikee wdyt?

src/fireEvent.ts

src/__tests__/questionsBoard.test.tsx

src/__tests__/fireEvent.test.tsx

AugustinLF · 2021-12-22T13:06:01Z

typings/test_index.js

@@ -11,15 +10,15 @@ import {
  within,
  getQueriesForElement,
  getDefaultNormalizer,
-} from '../..';
+} from '@testing-library/react-native';


@thymikee you suggested to export flow types the same way we did with TS, but I can't get the import here to work (hence the red build), are you familiar with how to do that?

AugustinLF · 2022-01-12T15:05:29Z

@thymikee I just rebased to fix the conflict, and I believe I addressed all the comments (only left one is mine #881 (comment) waiting for your input). Is there anything else you'd like me to do?

thymikee · 2022-01-12T15:57:04Z

@AugustinLF I see that Flow check is failing, would be cool to remove it :) Other than that please just wait a bit more for me to find time and review this. Thanks so much for the work you've done already!

AugustinLF · 2022-01-12T15:59:46Z

Yeah, the issue with the flow check is that I don't know how to make it possible to "test" the flow types #881 (comment). If I remove the test file for the types the build pass, but then we have no guarantee that the flow types are correct.

Thanks for taking the time reviewing :)

src/__tests__/a11yAPI.test.tsx

Killavus · 2022-01-25T22:16:30Z

src/__tests__/a11yAPI.test.tsx

-  expect(getAllByA11yLabel).toBe(getAllByLabelText);
-  expect(queryAllByA11yLabel).toBe(queryAllByLabelText);
-  expect(findAllByA11yLabel).toBe(findAllByLabelText);
-});


While we may discuss about usefulness of this test, why it gets removed with this PR? :)

This test just checks that aliases work and we removed them

#881 (comment)

Killavus · 2022-01-25T22:20:16Z

src/__tests__/act.test.tsx

 import * as React from 'react';
 import { Text } from 'react-native';
 import ReactTestRenderer from 'react-test-renderer';
 import act from '../act';
 import render from '../render';
 import fireEvent from '../fireEvent';

-const UseEffect = ({ callback }: { callback: Function }) => {
+type UseEffectProps = { callback: () => void };


small improvement (?):

Suggested change

type UseEffectProps = { callback: () => void };

type UseEffectProps = { callback(): void };

Killavus · 2022-01-25T22:28:12Z

src/__tests__/auto-cleanup-skip.test.tsx

@@ -11,7 +10,7 @@ beforeAll(() => {

 let isMounted = false;

-class Test extends React.Component<*> {
+class Test extends React.Component<any> {


Suggested change

class Test extends React.Component<any> {

class Test extends React.Component {

Killavus · 2022-01-25T22:29:41Z

src/__tests__/auto-cleanup.test.tsx

 import * as React from 'react';
 import { View } from 'react-native';
 import { render } from '..';

 let isMounted = false;

-class Test extends React.Component<*> {
+class Test extends React.Component<{ onUnmount?: () => void }> {


Here you can also do the same treatment onUnmount?(): void or keep it as-is. Decision is yours, I find the latter a little bit easier to read.

src/__tests__/byText.test.tsx

Killavus · 2022-01-25T22:35:10Z

src/__tests__/byText.test.tsx

@@ -146,7 +148,8 @@ describe('findBy options deprecations', () => {
 });

 test.skip('getByText works properly with custom text component', () => {
-  function BoldText({ children }) {
+  type BoldTextProps = { children: React.ReactNode };


How about extracting this to higher scope:

type ChildrenProps = { children: React.ReactNode }

And then reuse for all tests expecting children?

src/__tests__/timerUtils.ts

Killavus · 2022-01-25T22:54:32Z

src/flushMicroTasks.ts

+export function flushMicroTasks<T>(): Thenable<T> {
+  return {
+    // using "thenable" instead of a Promise, because otherwise it breaks when
+    // using "modern" fake timers


Can you check if it's still the case with TypeScript bindings for jest (@types/jest)? Keeping this type may be not optimal if it's not needed.

It's not a type issue (btw @types/jest are already installed), that comment/code was already there.

Looks mostly good to me. I'd definitely take a look whether we can get rid of Thenable in https://github.com/callstack/react-native-testing-library/pull/881/files#r792187961 flushMicroTasks after migration from Flow to TypeScript.

Sounds like a good plan to me.

Killavus · 2022-01-25T22:58:46Z

src/helpers/byDisplayValue.ts

-  value,
-  options?: TextMatchOptions = {}
+  node: ReactTestInstance,
+  value: string | RegExp,


I think this particular union may deserve an alias because of its widespread use. WDYT @thymikee?

Killavus · 2022-01-25T23:04:14Z

src/helpers/byText.ts

-const getChildrenAsText = (children, TextComponent) => {
-  const textContent = [];
+const getChildrenAsText = (
+  children: (string | number | React.ReactElement)[],


Suggested change

children: (string | number | React.ReactElement)[],

children: React.ReactChild[],

src/helpers/errors.ts

Killavus · 2022-01-25T23:49:44Z

src/helpers/timers.ts

@@ -30,20 +28,21 @@ function getJestFakeTimersType() {
  }

  if (
+    // @ts-ignore jest mutates setTimeout


I think it's a good place to use @ts-expect-error instead of @ts-ignore. We know that there'll be TS error here so no error is an undesired behavior as well.

Killavus · 2022-01-25T23:56:37Z

src/waitFor.ts

  await act(async () => {
    result = await waitForInternal(expectation, optionsWithStackTrace);
  });

-  //$FlowFixMe: either we have result or `waitFor` threw error
+  // @ts-ignore either we have result or `waitFor` threw error
  return result;


I think you can use ! operator to force TS to trust you it's always valid. You can keep the comment though.

Killavus

Looks mostly good to me. I'd definitely take a look whether we can get rid of Thenable in https://github.com/callstack/react-native-testing-library/pull/881/files#r792187961 flushMicroTasks after migration from Flow to TypeScript.

Also string | RegExp type may be aliased to something more in testing-library nomenclature. The rest of my comments are small stylistic improvements & small type improvements (like using React.ReactChild instead of enumerating possibilities, or restricting any in some places).

AugustinLF · 2022-02-15T10:04:27Z

@Killavus I should have addressed all your comments (or answered). I just let one hanging, waiting for a confirmation. Please let me know if you need anything else!

AugustinLF · 2022-03-02T13:38:14Z

@Killavus can I be of any help, or do anything to help getting that merged?

thymikee

Looks good and ready to be merged. However, since this is a breaking change, I wonder if we shouldn't tackle #934 first and release it in a minor release. WDYT?

AugustinLF · 2022-03-24T14:48:07Z

Sounds fair to me!

thymikee · 2022-03-25T09:32:43Z

@AugustinLF v9.1.0 is released so let's rebase this and we can merge this work 🚀

AugustinLF · 2022-03-25T13:16:15Z

@thymikee rebased. However CI didn't not thing to have been triggered, did you change anything recently?

thymikee · 2022-03-25T13:25:13Z

@AugustinLF nothing else than merging a bunch of Dependabot PRs

thymikee · 2022-03-25T13:31:14Z

There's some outage around self-hosted runners, may be related: https://status.circleci.com

src/helpers/timers.ts

thymikee · 2022-03-25T15:54:44Z

Thank you @AugustinLF!

AugustinLF commented Dec 6, 2021

View reviewed changes

AugustinLF force-pushed the convert-to-typescript branch from c5bdbec to f863bf1 Compare December 6, 2021 15:45

thymikee requested a review from mdjastrzebski December 9, 2021 12:13

mdjastrzebski reviewed Dec 10, 2021

View reviewed changes

AugustinLF commented Dec 22, 2021

View reviewed changes

AugustinLF force-pushed the convert-to-typescript branch from f99fddb to fa9a1c5 Compare January 12, 2022 14:48